-
Notifications
You must be signed in to change notification settings - Fork 36
Reconnect socket && worker ping #600
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
13f02bb
to
8fda0ae
Compare
Signed-off-by: elestrias <[email protected]>
8fda0ae
to
0ace090
Compare
Signed-off-by: elestrias <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #600 +/- ##
==========================================
- Coverage 46.02% 45.98% -0.04%
==========================================
Files 715 715
Lines 32328 32382 +54
Branches 17885 17918 +33
==========================================
+ Hits 14880 14892 +12
- Misses 13102 13140 +38
- Partials 4346 4350 +4
Continue to review full report at Codecov.
|
Signed-off-by: elestrias <[email protected]>
Signed-off-by: elestrias <[email protected]>
The problem I could see is that with connection retry we can connect to that socket with same host:port but it won't be the same process f e, instead of worker with gpu will be connected worker without gpu, problem is that we don't make changes in miner's worker info |
Signed-off-by: elestrias <[email protected]>
b3d5161
to
5c5067a
Compare
Signed-off-by: elestrias <[email protected]>
Signed-off-by: elestrias <[email protected]>
…ct/cpp-filecoin into RemoteWorkerPingRecconect
Signed-off-by: elestrias <[email protected]>
…ct/cpp-filecoin into RemoteWorkerPingRecconect
@@ -87,27 +88,28 @@ namespace fc::api::rpc { | |||
} | |||
} | |||
chans.clear(); | |||
reconnect(3, std::chrono::seconds(10)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use named variable or const instead of magic number 3
socket->async_write(boost::asio::buffer(buffer.data(), buffer.size()), | ||
[=](auto &&ec, auto) { | ||
std::lock_guard lock{mutex}; | ||
writing = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you change writing
under the guard, but it is read and changed above without any guard. If you want to avoid race condition in multithreading, please use more consistent approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moreover, you provide data by copy [=]
, not by refs. This change doesn't make any sense in this case.
@@ -185,4 +187,36 @@ namespace fc::api::rpc { | |||
} | |||
} | |||
} | |||
|
|||
void Client::reconnect(int counter, std::chrono::milliseconds wait) { | |||
if (reconnecting) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it thread-safe?
logger_->info( | ||
"Starting reconnect to {}:{}", client_data.host, client_data.port); | ||
for (int i = 0; i < counter; i++) { | ||
std::this_thread::sleep_for(wait*(i+1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::this_thread::sleep_for(wait*(i+1)); | |
std::this_thread::sleep_for(wait * (i + 1)); |
@@ -6,6 +6,7 @@ | |||
#include "remote_worker/remote_worker_api.hpp" | |||
|
|||
namespace fc::remote_worker { | |||
using api::ApiVersion; | |||
using api::VersionResult; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not used anymore, remove
using api::VersionResult; |
} | ||
void LocalWorker::ping(std::function<void(const bool resp)> cb) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
void LocalWorker::ping(std::function<void(const bool resp)> cb) { | |
} | |
void LocalWorker::ping(std::function<void(const bool resp)> cb) { |
Signed-off-by: elestrias [email protected]
Description of the Change
Benefits
Possible Drawbacks
Usage Examples or Tests [optional]
Alternate Designs [optional]